fix: close FAILED→COMPENSATING race in saga startup#201
Conversation
Move _mark_run_compensating earlier in start_compensation — right after the cheap in-memory eligibility check instead of after the storage-heavy _fetch_compensable_nodes call. On Windows with SQLite under contention, the storage I/O gap let wait()'s poll see the transient FAILED state as terminal.
📝 WalkthroughWalkthroughThe PR optimizes the saga compensation flow by reordering state transitions in ChangesCompensation Flow Optimization
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
py_src/taskito/workflows/saga/orchestrator.py (1)
512-521: 💤 Low valueMinor observation on the
already_doneassumption.The comment accurately describes the primary case (vacuous compensation), but the check could also trigger if non-vacuous compensation finishes between
start_compensationreturning and this lock acquisition. In that edge case, passingTruewould be incorrect if the child saga actually failed.This race window is extremely narrow (nanoseconds, bounded by GIL), making it practically impossible for asynchronous job execution/completion to occur in that time. The fix correctly addresses the real CI failure and the logic is sound for the documented vacuous case.
No action required—just noting the implicit assumption that
already_done=Trueimmediately afterstart_compensationimplies vacuous (always-success) completion.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@py_src/taskito/workflows/saga/orchestrator.py` around lines 512 - 521, The code assumes already_done implies vacuous (successful) compensation and calls _on_child_saga_terminal(child_run_id, True, None); instead, avoid passing True unconditionally—check the child's actual terminal state (e.g., consult a terminal-status map or failure set such as self._run_results / self._terminal_status or similar) before deciding the success boolean, and if no definitive status exists pass None so _on_child_saga_terminal can inspect authoritative state; update the branch around already_done and calls to _on_child_saga_terminal (and any helpers that record terminal outcomes) to derive the success flag from the real child terminal record rather than assuming success after start_compensation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@py_src/taskito/workflows/saga/orchestrator.py`:
- Around line 512-521: The code assumes already_done implies vacuous
(successful) compensation and calls _on_child_saga_terminal(child_run_id, True,
None); instead, avoid passing True unconditionally—check the child's actual
terminal state (e.g., consult a terminal-status map or failure set such as
self._run_results / self._terminal_status or similar) before deciding the
success boolean, and if no definitive status exists pass None so
_on_child_saga_terminal can inspect authoritative state; update the branch
around already_done and calls to _on_child_saga_terminal (and any helpers that
record terminal outcomes) to derive the success flag from the real child
terminal record rather than assuming success after start_compensation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a278b13d-b29b-420e-98b9-9691d1e44549
📒 Files selected for processing (1)
py_src/taskito/workflows/saga/orchestrator.py
1 participant
Summary
_mark_run_compensatingearlier instart_compensation— immediately after the cheap in-memory eligibility check, before the storage-heavy_fetch_compensable_nodescallmark_workflow_node_resultsetting the run to FAILED and the saga transitioning it to COMPENSATING exceeded the 100mswait()poll interval, causing the poll to see FAILED (terminal) and return earlyFixes CI failure:
test_child_saga_propagates_when_parent_node_is_sub_workflowonwindows-latest / Python 3.10Test plan
Summary by CodeRabbit
Bug Fixes
Performance